Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add SiteURI class #7252

Merged
merged 31 commits into from
Jul 11, 2023
Merged

feat: add SiteURI class #7252

merged 31 commits into from
Jul 11, 2023

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Feb 14, 2023

Description
This PR is needed for #7123

We need two URI classes. One for general purpose URI class, and the other is for URI of the app site.

  • add SiteURI class
    • The $routePath never starts with /.
    • If the path ends with /, the URI retains the trailing /.
    • getPath() returns the full URI path.
    • getRoutePath() returns the path relative to baseURL.

Related #5930, #7239, #7249, #7251

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@kenjis kenjis added enhancement PRs that improve existing functionalities 4.4 labels Feb 14, 2023
@kenjis kenjis force-pushed the feat-SiteURL branch 2 times, most recently from 2d602ea to 87c90ca Compare February 14, 2023 08:48
system/HTTP/SiteURI.php Outdated Show resolved Hide resolved
@michalsn
Copy link
Member

Maybe this is a silly question, but... as I understand, this new class will bring full PSR-7 compatibility, right?

I don't see any related changes in the framework so we won't rely on it. The question is, who needs this class and for what?

Like... I saw your description:

We need two URI classes. One for general purpose URI class, and the other is for URI of the app site.

But I'm afraid I don't fully understand the use cases.

@kenjis
Copy link
Member Author

kenjis commented Feb 15, 2023

This PR only adds a new SiteURI class, and does not change the current framework behavior yet.

In the next PR(s), I will do:

  • add a factory to create the SiteURI instance for the current URL: feat: add SiteURIFactory #7256
  • set the SiteURI instance in the uri property of the Request
  • use the SiteURI instance when getting the current URL like current_url()

About compatibility with PSR-7, the URI class will be cleaner because site-related functions like baseURL, will be separated into the SiteURI class. And the URI class will be easier to change.

Finally, I think URI and SiteURL can be made fully compatible (and having some extended functions) with PSR-7.

@kenjis kenjis mentioned this pull request Feb 15, 2023
5 tasks
Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am obviously in favor of this but I want to be sure we get reviews from @lonnieezell and @iRedds and anyone else who was opposed to the idea originally.

@lonnieezell
Copy link
Member

lonnieezell commented Feb 15, 2023

I was purposefully not commenting since my position was already known and it seemed I was overruled :)

I'm confused why we need a second class that does 99% of the same things as the URI class. The URI class is just for working with URIs in general, crafted to meet the RFC as best we could at the time. It seems like all other issues should be fixed where it's used, or the base class should be changed to support our needs.

The implementation itself is solid enough. Though if we're going to include it, we need docs and guidance on when to use which one.

It seems strange to me to make this new class and we're not using it ourselves in IncomingRequest or the URL helpers. What is the point of this other then to throw confusion about which class people should use when? I figured it was to fix issues in how the framework was returning answers, but it doesn't seem like we're actually doing that.

@michalsn
Copy link
Member

I agree with @lonnieezell. I don't understand why we need a second class to handle URLs. Can somebody give me a clear example of where it will be useful?

AFAIK the only issue that we have now with the URI class is that it does not follow PSR-7 (it's something path related).

Also, if the goal for both classes is to follow the PSR-7, what will be the difference? Wouldn't it be better to see some additional methods added to the current class instead?

@kenjis
Copy link
Member Author

kenjis commented Feb 16, 2023

I don't understand why we need a second class to handle URLs.

Because we have already two kinds of concept URI,
and if we have two classes, It is easier to understand for us.

  1. URI: a URI
  2. SiteURI: a URI for a CI4 app site.

SiteURI has its own properties like baseURL, indexPage and we cannot change the URI path
inside baseURL (subfolders). Only the URI path after indexPage does matter.

Now we are already confusing 1. and 2. So we have two kinds of URI segments for one SiteURL.
See #7123 (comment)

@kenjis
Copy link
Member Author

kenjis commented Feb 16, 2023

It seems strange to me to make this new class and we're not using it ourselves in IncomingRequest or the URL helpers.

This fix (and refactoring) will be long story. This PR is just the first one.
We will use the new class in all places after all. Please wait for incoming PRs.

@MGatner
Copy link
Member

MGatner commented Feb 16, 2023

I won't repeat my arguments, the original PR is here with discussion: #4647

I would like to point out that after doing a deep dive on URI handling and bug fixes both @kenjis and I came to the same conclusion, independently. I think the problem is larger than credited and may indeed merit the potential confusion/clutter of an additional class.

@lonnieezell
Copy link
Member

After giving this a bit more thought, I think I'm ok with this on 2 conditions:

  1. It's designed for internal use within the request class and URL helper functions. Not intended for public use or really even documented. This avoids the confusion to end users, while still providing a "URI in Request is the source of truth" that @MGatner was looking for in the previous PR.
  2. It's saved for 5.0 (which we should be working on now....) since it's too big of a potential BC break.

@kenjis kenjis force-pushed the feat-SiteURL branch 2 times, most recently from f65a55c to 72f76a6 Compare February 17, 2023 01:04
@iRedds
Copy link
Collaborator

iRedds commented Feb 17, 2023

$uri = $this->request->getUri();
echo (string) $uri;   // "http://localhost:8888/ci431/public/test?a=b" → Okay
echo $uri->getPath(); // "test" → NG. It should be "/ci431/public/test" when following PSR-7

And I think that such behavior corresponds to PSR.

// for http://localhost:8888/ci431/public/test?a=b
URI::getPath(); // => /ci431/public/test

// for http://localhost:8888/{base_path}/test?a=b
URI::getPath(); // => test    (without leading slash, as an indicator that the base path is being used.)

// Accordingly, the leading slash will determine whether the path is absolute or rootless

// for http://localhost:8888/{base_path}/test?a=b
URI::withPath('/xxx'); // http://localhost:8888/xxx?a=b

// for http://localhost:8888/{base_path}/test?a=b
URI::withPath('xxx'); // http://localhost:8888/{base_path}/xxx?a=b

php-fig/fig-standards#503

@kenjis
Copy link
Member Author

kenjis commented Feb 17, 2023

What is {base_path} in this case? How do you define it?

// for http://localhost:8888/{base_path}/test?a=b
URI::getPath(); // => test    (without leading slash, as an indicator that the base path is being used.)

@iRedds
Copy link
Collaborator

iRedds commented Feb 17, 2023

@kenjis app.baseURL = 'http://localhost:8888{/ci431/public/}' <- base path

@kenjis
Copy link
Member Author

kenjis commented Feb 17, 2023

I don't get it.
How do you set it in PSR-7 URI object?

@iRedds
Copy link
Collaborator

iRedds commented Feb 17, 2023

There are no objects in PSR-7. Interfaces only. That is, mandatory public methods for PSR compliance.
A class implementing UriInterface should not be limited to UriInterface methods and may contain a method that sets base_path or base_url or whatever is needed for it to work properly.

@kenjis
Copy link
Member Author

kenjis commented Feb 17, 2023

Many frameworks provide the ability to get the "base path," usually considered the path up to and including the front controller. As an example, if the application is served at http://example.com/b2b/index.php, and the current URI used to request it is http://example.com/b2b/index.php/customer/register, the functionality to retrieve the base path would return /b2b/index.php. This value can then be used by routers to strip that path segment prior to attempting a match.
https://www.php-fig.org/psr/psr-7/meta/#why-is-no-functionality-included-for-retrieving-the-base-path

 * If an HTTP path is intended to be host-relative rather than path-relative
 * then it must begin with a slash ("/"). HTTP paths not starting with a slash
 * are assumed to be relative to some base path known to the application or
 * consumer.
   ...
 */
public function withPath($path);

https://www.php-fig.org/psr/psr-7/

This means like this?

  • if we call URI::withPath('foo/bar'), the path foo/bar is relative to some base path known to the application.
  • the new URI instance getPath() will return foo/bar?

@kenjis
Copy link
Member Author

kenjis commented Feb 17, 2023

<?php

require __DIR__ . '/vendor/autoload.php';

use League\Uri\Http;

$uri = Http::createFromBaseUri('foo/bar', 'http://localhost/ci4/');
echo $uri . PHP_EOL;            // http://localhost/ci4/foo/bar
echo $uri->getPath() . PHP_EOL; // /ci4/foo/bar

https://uri.thephpleague.com/uri/6.0/psr7/

@kenjis
Copy link
Member Author

kenjis commented Feb 17, 2023

use League\Uri\Http;

$uri = Http::createFromBaseUri('foo/bar', 'http://localhost/ci4/');
$uri2 = $uri->withPath('controller/method');

PHP Fatal error: Uncaught League\Uri\Exceptions\SyntaxError: If an authority is present the path must be empty or start with a /. in /.../league-uri/vendor/league/uri/src/Uri.php:921

@kenjis
Copy link
Member Author

kenjis commented Feb 17, 2023

use Slim\Psr7\Factory\UriFactory;

$factory = new UriFactory();

$uri = $factory->createUri('http://localhost/ci4/');
$uri2 = $uri->withPath('controller/method');
echo $uri2 . PHP_EOL;            // http://localhost/controller/method
echo $uri2->getPath() . PHP_EOL; // controller/method

$uri2 = $uri->withPath('/controller/method');
echo $uri2 . PHP_EOL;            // http://localhost/controller/method
echo $uri2->getPath() . PHP_EOL; // /controller/method

@kenjis
Copy link
Member Author

kenjis commented Feb 17, 2023

use Nyholm\Psr7\Uri;

$uri = new Uri('http://localhost/ci4/');
$uri2 = $uri->withPath('controller/method');
echo $uri2 . PHP_EOL;            // http://localhost/controller/method
echo $uri2->getPath() . PHP_EOL; // controller/method

$uri2 = $uri->withPath('/controller/method');
echo $uri2 . PHP_EOL;            // http://localhost/controller/method
echo $uri2->getPath() . PHP_EOL; // /controller/method

@iRedds
Copy link
Collaborator

iRedds commented Feb 17, 2023

  • the new URI instance getPath() will return foo/bar?

empty string

Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! I feel validated in the need for this given how much code is in SiteURI on top of URI. I appreciate your verbosity in naming variables and methods - it is easy to overlook pieces which has led to many bugs in the past. I think this will be a good step forward.

I assume the URL Helper methods get changed in the next PR? I haven't looked at that one yet...

system/HTTP/SiteURI.php Outdated Show resolved Hide resolved
system/HTTP/SiteURI.php Outdated Show resolved Hide resolved
system/HTTP/SiteURI.php Show resolved Hide resolved
system/HTTP/URI.php Outdated Show resolved Hide resolved
system/HTTP/URI.php Outdated Show resolved Hide resolved
@kenjis kenjis merged commit 09f62eb into codeigniter4:4.4 Jul 11, 2023
@kenjis kenjis deleted the feat-SiteURL branch July 11, 2023 23:14
@kenjis
Copy link
Member Author

kenjis commented Jul 11, 2023

@TimexPeachtree @MGatner Thank you for approvals.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.4 enhancement PRs that improve existing functionalities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants